Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add advanced code example to documentation #3026

Merged
merged 8 commits into from
Nov 15, 2023
Merged

Conversation

Alex1607
Copy link
Contributor

@Alex1607 Alex1607 commented Apr 11, 2023

What this PR solves / how to test:
In my opinion, removing the code example and just using a bare-bones file leaves a lot of new starters in the dark. Therefore, I added the old code as an advanced example to the documentation.

Associated docs issue(s)/PR(s):

Author has included the following, where applicable:

Reviewer has performed the following, where applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

@Alex1607 Alex1607 requested a review from a team as a code owner April 11, 2023 23:06
@changeset-bot
Copy link

changeset-bot bot commented Apr 11, 2023

⚠️ No Changeset found

Latest commit: d25a499

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Apr 11, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6882249059/npm-package-wrangler-3026

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6882249059/npm-package-wrangler-3026

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6882249059/npm-package-wrangler-3026 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6882249059/npm-package-miniflare-3026
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6882249059/npm-package-cloudflare-pages-shared-3026

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.15.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20231025.1
workerd 1.20231030.0 1.20231030.0
workerd --version 1.20231030.0 2023-10-30

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #3026 (d25a499) into main (3753eaf) will increase coverage by 0.03%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3026      +/-   ##
==========================================
+ Coverage   75.46%   75.50%   +0.03%     
==========================================
  Files         225      225              
  Lines       12478    12478              
  Branches     3239     3239              
==========================================
+ Hits         9417     9421       +4     
+ Misses       3061     3057       -4     

see 3 files with indirect coverage changes

@JacobMGEvans
Copy link
Contributor

Please run Prettier
npm run fix

Copy link
Contributor

@JacobMGEvans JacobMGEvans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The messaging needs some rewording and the formatter needs to be ran on the file. If the formatter makes the snippet look off we can add that to the prettier/linter ignore

templates/experimental/worker-rust/README.md Outdated Show resolved Hide resolved
Alex1607 and others added 3 commits April 12, 2023 18:09
Co-authored-by: Jacob M-G Evans <27247160+JacobMGEvans@users.noreply.github.com>
@Alex1607
Copy link
Contributor Author

Checks are failing because a test timed out: → Test timed out in 30000ms.
I don't think it's related to my changes.

@ghost
Copy link

ghost commented May 3, 2023

I suggest we use example from the workers-rs repo, it's compact and does the job.

use worker::*;

#[event(fetch)]
pub async fn main(req: Request, env: Env, _ctx: worker::Context) -> Result<Response> {
    console_log!(
        "{} {}, located at: {:?}, within: {}",
        req.method().to_string(),
        req.path(),
        req.cf().coordinates().unwrap_or_default(),
        req.cf().region().unwrap_or("unknown region".into())
    );

    if !matches!(req.method(), Method::Post) {
        return Response::error("Method Not Allowed", 405);
    }

    if let Some(file) = req.form_data().await?.get("file") {
        return match file {
            FormEntry::File(buf) => {
                Response::ok(&format!("size = {}", buf.bytes().await?.len()))
            }
            _ => Response::error("`file` part of POST form must be a file", 400),
        };
    }

    Response::error("Bad Request", 400)
}

@mrbbot
Copy link
Contributor

mrbbot commented Aug 21, 2023

@JacobMGEvans are you happy with these changes? If so, would you be able to approve and merge?

@jspspike
Copy link
Contributor

@Alex1607 Could this link to workers-rs README? Since this is just a copy of that I feel like that would make sense in the case the reader wants to read more or any changes are made to this example based on changes in workers-rs. Honestly I feel like it would make the most sense to remove the snippet entirely and just say you could go to this link for more info but if you feel like it makes sense to include it that's fine with me

@lrapoport-cf lrapoport-cf added the awaiting reporter response Needs clarification or followup from OP label Oct 23, 2023
@lrapoport-cf
Copy link
Contributor

hi @Alex1607 :) could you please note that the snippet comes from workers-rs and include a link, and comment that for up to date info users should follow the link? thanks!

@Alex1607
Copy link
Contributor Author

@jspspike @lrapoport-cf sorry for the delay, I just adjusted the readme

Copy link
Contributor

@jspspike jspspike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@Alex1607
Copy link
Contributor Author

How is the merging done? Do I still have to do something, or will someone take over from now?

@lrapoport-cf lrapoport-cf merged commit 46a35e0 into cloudflare:main Nov 15, 2023
17 checks passed
@lrapoport-cf
Copy link
Contributor

thanks @Alex1607 , we've merged the change 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting reporter response Needs clarification or followup from OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants